[Shopify] Automatic Transaction Posting - #9525
[Shopify] Automatic Transaction Posting#9525Onat Buyukakkus (onbuyuka) wants to merge 17 commits into
Conversation
Automatically post Shopify order and refund payment transactions as general journal lines when the related sales invoice or credit memo is posted, when the transaction's payment method mapping is configured for automatic posting. Posting is synchronous and best-effort: a failure to post a payment is logged as a Shopify skipped record and never blocks or reverses the document posting. Preview posting and commit-suppressed postings are respected (auto-posting is skipped in those cases). Fixes AB#620951 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42e38781-540d-47bf-8a06-86ee9aceb050
…620951-shopify-automatic-transaction-posting
…620951-shopify-automatic-transaction-posting
…620951-shopify-automatic-transaction-posting
|
The new ShowPostableTransactions and ClearFilter actions are promoted into the Related group, but Related is reserved for record-linked navigation (e.g., Customer Ledger Entries) while view-filter actions like these fit the standard Process group instead. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4 |
|
The best-effort auto-post path only traps AutoGenJnlPost.Run(...) and GenJnlPostBatch.Run(...) via their boolean return values. The surrounding RemoveJournalLines(...), the post-build Commit(), and LogFailureAndCommit(...) still raise normally on failure, so an exception there would escape OnAfterPostSalesDoc even though the whole feature is designed to never interrupt document posting. Additionally, if an exception occurs after BindSubscription(AutoGenJnlPost) but before the corresponding UnbindSubscription call (e.g. inside RemoveJournalLines before Run, or inside the post-build Commit before GenJnlPostBatch.Run), the manual event subscriber instance is left bound for later, unrelated journal postings in the same session. Wrap the whole attempt so cleanup/commit/logging cannot itself abort the caller, and guarantee UnbindSubscription runs on every exit path (including exceptional ones). Agent judgement — not directly backed by a BCQuality knowledge article. Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4 |
|
PostTransactions iterates Shopify order/refund transactions with FindSet/repeat and, for each row whose payment method mapping enables auto-posting, calls PostTransaction which itself issues Commit() (once to establish a rollback boundary before the first payment, again after building each journal line before batch posting, and again in LogFailureAndCommit on failure). When an invoice or credit memo carries multiple transactions, this produces one journal batch posting (and one or more commits) per transaction instead of one combined operation, which is the per-row commit anti-pattern this article documents. The design intentionally isolates a failed payment posting from already-succeeded ones and from the underlying document post, which is a legitimate trade-off, but it is worth the author confirming the extra commit/posting-batch overhead per transaction is acceptable for orders with many line-item transactions. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4 |
|
MarkPostableTransactions filters Shpfy Order Transaction by Shop, Gateway, and Credit Card Company, but the table's only keys are Shopify Transaction Id (clustered), Gift Card Id, Created At, and Type — none start with Shop/Gateway/Credit Card Company. FilterPostableTransactions calls this once per auto-post-enabled payment mapping (in a repeat/until loop), so each call performs a filtered scan with no supporting key, and the cost multiplies by the number of configured mappings. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4 |
|
UnitTestAutoPostJnlBatchValidateWithoutBalAccountNo uses a bare Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4 |
|
The PR adds the "Auto-Post Enabled" transactions-page field plus the new Filter Postable Transactions/Clear Filter action flow, but there is no page test that opens Shpfy Transactions, runs the filter dialog, and asserts which records remain marked. Add a UI test covering the gateway/date filters and the Clear Filter action so regressions in this new filtering surface are caught. Agent judgement — not directly backed by a BCQuality knowledge article. Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4 |
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Request changes:
S1 - Automatic posting can post unrelated journal lines
Shpfy Auto Post Transactions filters the journal line record to the configured template and batch, then explicitly clears the Shpfy Transaction Id filter before calling Gen. Jnl.-Post Batch. This posts the entire configured batch, including unrelated pre-existing manual journal lines. The setup does not require or enforce a dedicated empty batch, so posting a sales invoice can unexpectedly post entries the user did not intend to post.
Please isolate automatic lines in a dedicated batch or use a posting path that is scoped to only the generated transaction lines. Add a regression test that places an unrelated line in the configured batch and verifies it remains unposted.
S2 - Partial invoicing can consume the full Shopify transaction too early
Automatic posting runs after each invoice is posted, while Shpfy Suggest Payments distributes the full order transaction over invoices that exist at that moment and creates a G/L residual for any remaining amount. For a split or partially invoiced Shopify order, the first invoice can therefore consume and mark the whole transaction as used before later invoices are posted, leaving later invoices unpaid or misallocating the remainder.
Please add split/partial-invoice coverage and ensure the first invoice does not consume the portion belonging to invoices that have not yet been posted.
S3 - The “postable transactions” filter does not match posting eligibility
The filter checks only Used = false, a posted invoice number, and a mapping with Post Automatically = true. It does not enforce the automatic-posting routine's Status = Success, supported transaction type, or non-empty journal template/batch requirements, so pending, failed, authorization, or incompletely configured transactions can be shown as postable. The end-date range also ends at 00:00, excluding nearly the entire selected end date.
Please align the UI filter with the actual posting predicates and make the selected end date inclusive.
- S1: post each transaction through a dedicated single-use journal batch cloned from the configured one, so unrelated lines parked in the configured batch are never posted. - S2: defer auto-posting while other unposted sales documents exist for the same Shopify order/refund, so a partial invoice can't consume the whole transaction. - S3: align the "Filter Postable Transactions" list with the posting eligibility predicates and make the selected end date inclusive. - Clear the auto-post batch on any journal template change. - Move batch creation and line building into the runner's OnRun to avoid the INSERT-in-TryFunction restriction; bind the working-date subscriber once per document with a guaranteed unbind. - Add tests for batch isolation, partial-invoice deferral and journal parameter propagation; renumber the test codeunit to 139587. Fixes AB#620951 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42e38781-540d-47bf-8a06-86ee9aceb050
Round 2 — review feedback addressed (commit 34eab3e)Thanks for the detailed review. Summary of the changes. Predrag Maricic (@PredragMaricic)S1 — automatic posting could post unrelated journal lines. Each transaction is now posted through a dedicated, single-use batch ( S2 — partial invoicing consuming the full transaction too early. Auto-posting now defers while any not-yet-posted sales document exists for the same Shopify order/refund ( S3 — filter vs. posting eligibility + end date. The "Filter Postable Transactions" list now enforces the same predicates as the posting routine ( AL review agent — inline threads (resolved)
AL review agent — general comments
All tests green: 15/15 auto-post + 9/9 Suggest Payment regression. App builds clean (0 errors / 0 warnings). |
AI PR Review — Round 1Recommendation: Accept with Suggestions Risk assessment: The automatic posting path is isolated and best-effort, but the new postable-transaction filter does not mirror the refund flow. Findings / suggestions🟠 S1 — Align the postable filter with refund posting readiness
Review mode: autonomous conversation comment (
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cef4fb6d-f5f4-46b3-a408-783d786ea02c
Review follow-up —
|
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThis PR adds automatic posting of Shopify payment and refund transactions when the related invoice or credit memo is posted. The core posting design now uses a shared eligibility check, a dedicated single-use journal batch, and best-effort failure logging, which matches the intended feature shape. Status of previous suggestions
New observations (commits since round 1)S2 (🔴 High): Remove unused using directives Risk assessment and necessityRisk: This touches financial posting for Shopify invoices and refunds. The posting flow is isolated and has focused tests, but the current build failure blocks validation. Necessity: The feature is useful and scoped to automatic payment posting for mapped Shopify transactions. The previous refund-filter issue appears fixed, but the compile errors must be corrected.
|
…utomatic-transaction-posting
Resolves the PR review findings on automatic transaction posting: - Remove the two unused using directives that broke every app build (AL0792). - Perf: calculate the Used FlowField via SetAutoCalcFields on the eligibility callers instead of a per-row CalcFields inside the loop. - Privacy/telemetry: stop emitting raw error text/call stack; the finalization failure event now carries only an error code as SystemMetadata. - Drop the journal permission pre-check and the finalize codeunit's elevated Permissions property in favour of best-effort posting. - Revert Credit Card Company to Text[30] to avoid a primary-key width change on the released Shpfy Payment Method Mapping table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5155ee0a-3835-4415-9dc0-a79dfd96b734
7e2fbbc
| report "Shpfy Translator" = X, | ||
| codeunit "Company Details Checklist Item" = X, | ||
| codeunit "Shpfy Authentication Mgt." = X, | ||
| codeunit "Shpfy Auto Gen. Jnl.-Post" = X, |
There was a problem hiding this comment.
The new automatic-posting feature exposes general-journal setup on the Shopify payment-method mapping page and then creates/posts isolated general-journal batches during invoice/credit-memo posting, but none of the app's assignable Shopify permission sets (built on top of "Shpfy - Objects", which only grants execute permission on the new Shopify objects themselves) add the underlying Gen. Journal Line/Gen. Journal Batch table permissions the feature needs at setup and posting time. A user assigned only the app's Shopify roles cannot configure or exercise this feature without an additional, non-Shopify role, matching the AppSource anti-pattern of shipping a workflow with missing permission coverage.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
|
|
||
| internal procedure GetPaymentMethodMapping(OrderTransaction: Record "Shpfy Order Transaction"; var PaymentMethodMapping: Record "Shpfy Payment Method Mapping"): Boolean | ||
| begin | ||
| exit(PaymentMethodMapping.Get(OrderTransaction.Shop, OrderTransaction.Gateway, OrderTransaction."Credit Card Company")); |
There was a problem hiding this comment.
The new auto-post eligibility lookup keys into "Shpfy Payment Method Mapping" with OrderTransaction."Credit Card Company", but this feature relies on a data model where the transaction value is Text[50] while the mapping table stores the same key segment as Text[30]. Credit-card-company names longer than 30 characters can therefore exist on transactions but fail to match the mapping row, causing otherwise eligible transactions to be skipped. Align the field length across the transaction, mapping, and related lookup/master records before using this field as part of the auto-post key.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
| { | ||
| Access = Internal; | ||
|
|
||
| internal procedure AutoPostTransactions(SalesInvoiceHeaderNo: Code[20]; SalesCrMemoHeaderNo: Code[20]) |
There was a problem hiding this comment.
The new automatic-posting pipeline in Shpfy Auto Post Transactions is a posting entry point, but it introduces no OnBefore.../OnAfter... integration events around the core posting flow. That makes eligibility, journal construction, and failure handling a hard wall for extensions, forcing partners to copy or replace the feature instead of subscribing to thin hooks at the operation boundaries.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
| PostTransactions(OrderTransaction, SalesCrMemoHeader."Posting Date"); | ||
| end; | ||
|
|
||
| local procedure PostTransactions(var OrderTransaction: Record "Shpfy Order Transaction"; PostingDate: Date) |
There was a problem hiding this comment.
Shpfy Auto Post Transactions hardwires Shpfy Auto Post Eligibility, Shpfy Auto Gen. Jnl.-Post, and Shpfy Auto Post Finalize as concrete Codeunit collaborators. Because the posting and cleanup paths are invoked through concrete codeunits, tests cannot inject doubles to exercise the success, skip, and failure branches of the auto-post flow in isolation. Depend on interfaces and inject the implementations instead of constructing these collaborators directly.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
| SkippedRecord: Codeunit "Shpfy Skipped Record"; | ||
| begin | ||
| if Shop.Get(OrderTransaction.Shop) then | ||
| SkippedRecord.LogSkippedRecord(OrderTransaction."Shopify Transaction Id", OrderTransaction.RecordId, CopyStr(FailureReason, 1, 250), Shop); |
There was a problem hiding this comment.
This new skipped-record path persists parameterless GetLastErrorText() output into "Shpfy Skipped Record"."Skipped Reason". Unsanitized GetLastErrorText() can contain customer content, but "Skipped Reason" is a Normal table field with no explicit DataClassification, so the PR introduces customer-bearing data into an under-classified stored field.
Knowledge:
- microsoft/knowledge/privacy/data-classification-required-on-pii-fields.md
- microsoft/knowledge/privacy/table-level-data-classification-cascades.md
- microsoft/knowledge/privacy/getlasterrortext-customer-content-in-errors.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
Summary
Introduces automatic posting of Shopify order/refund payment transactions as general journal lines when the related invoice or credit memo is posted in Business Central. This is a reworked, hardened version of the feature originally proposed in #6515.
Changes
Automatic-posting setup
Post Automatically,Auto-Post Jnl. Template, andAuto-Post Jnl. Batchto payment-method mappings.Auto-Post Enabledtransaction FlowField aligned with the complete setup requirements.Posting and filtering
Posting safety
Codeunit.Runoperations.Tests
The automatic-posting test suite covers setup validation, Sale/Capture/Refund posting, multiple and mixed transactions, unrelated journal lines, partial invoices and credit memos, document-link transaction boundaries, future posting dates, suppressed commits, preview, job-queue setup, failure handling, parameter propagation, and shared postable eligibility.
The Shopify app builds successfully with the AL MCP server. The full local test-project build is currently blocked by the existing
MockAzureKeyVaultSecretProviderenvironment dependency; CI provides the full test matrix.Fixes AB#620951